Opened 8 years ago

Closed 8 years ago

#946 closed defect (fixed)

Bind error in Tunnel Manager when destination address is invalid

Reported by: DISABLED Owned by: zzz
Priority: minor Milestone: 0.9.8
Component: apps/i2ptunnel Version: 0.9.6
Keywords: Cc:
Parent Tickets: Sensitive: no


Steps to reproduce in i2ptunnel manager:

  • Create a standard tunnel with default settings. Make sure autostart is enabled
  • Include a bogus destination address - we use
  • Start the tunnel
  • Here confirm errors. No bind error is present. We know target destination is not found
  • Tunnel will be 'not started' a few moments after autostart failure - Start the tunnel now
  • Note bind errors on interface, address/port already in use

Eventually the tunnel starts and the bind error goes away.
I presume this is due to after initial failure bind is not destroyed correctly.


Change History (9)

comment:1 Changed 8 years ago by str4d

See apps/i2ptunnel/java/src/net/i2p/i2ptunnel/ lines 51-66:

if (dests.isEmpty()) {
    l.log("No valid target destinations found");
    notifyEvent("openClientResult", "error");
    // Nothing is listening for the above event, so it's useless
    // Maybe figure out where to put a waitEventValue("openClientResult") ??
    // In the meantime, let's do this the easy way
    // Note that b32 dests will often not be resolvable at instantiation time;
    // a delayed resolution system would be even better.

    // Don't close() here, because it does a removeSession() and then
    // TunnelController can't acquire() it to release() it.
    // Unfortunately, super() built the whole tunnel before we get here.
    throw new IllegalArgumentException("No valid target destinations found");

comment:2 Changed 8 years ago by str4d

FindBugs? doesn't like this either - http://jenkins.killyourtv.i2p/job/I2P/202/findbugsResult/file.147862710/, SC_START_IN_CTOR, Priority: High

SC: new net.i2p.i2ptunnel.I2PTunnelClientBase(int, Logging, I2PSocketManager, I2PTunnel, EventDispatcher?, long) invokes Thread.start()

The constructor starts a thread. This is likely to be wrong if the class is ever extended/subclassed, since the thread will be started before the subclass constructor is started.

Question is, what to do about it? At what stage should the local IP:port be set up, and by what? I see there are two almost-identical constructors in I2PTunnelClientBase, one of which respects a "delayOpen" parameter, but that only delays opening the I2P side of the tunnel.

comment:3 Changed 8 years ago by zzz

Related: #651 #722, possibly others. One of them mentions the start-thread-in-constructor issue.

If the tunnel is a shared client it may be even worse, see e.g. #815

I2PTunnelDCCClient delays resolution until connect time. I think at one point I intended on porting that "up" to I2PTunnelClient (and some of the other subclasses) but never did it.

Perhaps because maybe we do want to complain or fail if it doesn't resolve? But maybe only if it's a "real" hostname, but not a well-formed b32. So I guess I never did it because I never decided what to do exactly.

The thread issue is really bad but it may or may not be necessary to fix it to fix this ticket. I have 30 things in i2ptunnel.config, all but a couple are disabled, but we start a thread for each of them. The whole event and notify thing is pretty whacky.

comment:4 Changed 8 years ago by zzz

Owner: set to zzz
Status: newaccepted

The above comment isn't completely true, we don't start a thread for each tunnel anymore.

Working on fixing all this in the i2p.i2p.zzz.test2 branch.

comment:5 Changed 8 years ago by DISABLED

Resolution: worksforme
Status: acceptedclosed

comment:6 Changed 8 years ago by str4d

Resolution: worksforme
Status: closedreopened

comment:7 Changed 8 years ago by DISABLED

Resolution: duplicate
Status: reopenedclosed

comment:8 Changed 8 years ago by str4d

Resolution: duplicate
Status: closedreopened

comment:9 Changed 8 years ago by zzz

Resolution: fixed
Status: reopenedclosed

Fixed in standard and IRC client tunnels in 3ddedb08de8709fb784e8557b80feefe0d1ddf8a. Didn't bother to fix streamr client.

Unresolvable hostname spits out a warning but does not throw out of the constructor. An unresolved hostname will be re-resolved at connection time. This also fixes the issue with b32 hostnames. Removed GUI warning that they weren't recommended.

I opened #973 a few months ago for the generic start-threads-in-constructors issue raised in comments 2 and 3 above.

Note: See TracTickets for help on using tickets.